fix: NavigationBarDestination.selected_icon renders wrongly when set to a Control#6468
Merged
FeodorFitsner merged 22 commits intorelease/v0.85.0from May 6, 2026
Merged
Conversation
1 task
NavigationBarDestination.selected_icon renders wrongly when set to a ControlNavigationBarDestination.selected_icon renders wrongly when set to a Control
Deploying flet-website-v2 with
|
| Latest commit: |
dfcf014
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://42f0510d.flet-website-v2.pages.dev |
| Branch Preview URL: | https://fix-nav-destination-selected.flet-website-v2.pages.dev |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes #6460 where NavigationBarDestination.selected_icon rendered incorrectly when provided as an Icon control (e.g., ft.Icon(...)) by ensuring the Flutter-side implementation treats selected_icon consistently as “icon-or-widget” rather than always attempting to parse icon data.
Changes:
- Update
NavigationBarDestinationControlto buildselectedIconviacontrol.buildIconOrWidget("selected_icon"), avoidinggetIconData()on non-intvalues. - Adjust the Material
NavigationBarintegration screenshot test to select the destination that uses aControl-typedselected_icon. - Add a root changelog entry documenting the fix.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sdk/python/packages/flet/integration_tests/controls/material/test_navigation_bar.py | Updates screenshot coverage to exercise selected_icon when provided as an ft.Icon control (and ensures it’s visibly selected). |
| packages/flet/lib/src/controls/navigation_bar_destination.dart | Fixes selected icon rendering by using the shared buildIconOrWidget() path for selected_icon. |
| CHANGELOG.md | Documents the bugfix for NavigationBarDestination.selected_icon when provided as an Icon control. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Deploying flet-examples with
|
| Latest commit: |
dfcf014
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e8809b29.flet-examples.pages.dev |
| Branch Preview URL: | https://fix-nav-destination-selected.flet-examples.pages.dev |
1 task
FeodorFitsner
approved these changes
May 5, 2026
Avoids the `!debugNeedsPaint` assertion in RenderRepaintBoundary.toImage when the boundary is still dirty at capture time on slower runners. Also temporarily narrows macos-integration-tests workflow to examples/controls/core::test_row.py to validate the fix in CI.
The macOS runner upgrade (macos-14 → -15 → -26) increased the default viewport size, making the Row alignment screenshots flake with `!debugNeedsPaint` because the screenshot package's 20ms post-delay was no longer enough to flush a paint of the larger surface. Calling resize_page(800, 1000) restores deterministic paint workload. Reverts the earlier endOfFrame attempt, which deadlocks under the flutter_test fake scheduler.
1 task
The screenshot package waits 20ms before calling RenderRepaintBoundary.toImage(). On macos-26 runners that's too short — the capture invocation itself can schedule a frame that's still painting when toImage() runs, tripping the `!debugNeedsPaint` assertion. Pass an explicit 200ms delay from the testing harness for both take_page_controls_screenshot and assert_control_screenshot.
To capture more diagnostic output for the test_alignment screenshot race investigation. Revert before merge.
The OS window resize triggered by resize_page propagates asynchronously. Without an explicit settle, it races into the capture's post-delay and re-dirties the RepaintBoundary right before toImage() runs, tripping `!debugNeedsPaint`. CI debug log showed the window UPDATE_CONTROL_PROPS arriving 1ms after the capture invocation was sent.
The resize_page workaround introduced its own race: macOS window resize is OS-async beyond Flutter's pump cycle, the requested height is clamped to the runner's display, and the late-arriving resize event re-dirties the boundary anyway. Revert and rely on the 200ms capture delay only.
Use pump_times to outlast any scrollbar-fade or SafeArea inset adjustment that might keep the boundary dirty for ~1.5s. Diagnostic — narrowing down the !debugNeedsPaint cause.
The page is already scrollable; the inner Column.scroll=AUTO produces a double-scroll layout that combined with intrinsic_width wrapping in the test harness keeps the RepaintBoundary perpetually dirty, tripping !debugNeedsPaint when the screenshot is captured.
Drops the 200ms capture delay, pump_times=10 in test_alignment, and DEBUG-level pytest logging — none were the actual fix; the inner double-scroll in the alignment example was. Workflow stays narrowed to test_row.py for one more validation run before reverting that too.
PR #6450 wrapped the scrollable child in a LayoutBuilder to read viewport size for the min-height constraint. LayoutBuilder reports 0 for intrinsic dimensions, which collapses any ancestor IntrinsicWidth / IntrinsicHeight and leaves the layout in an unstable state — the test harness's intrinsic_width=True wrapper then perpetually marks the RepaintBoundary dirty, tripping !debugNeedsPaint when toImage() runs. MediaQuery.sizeOf exposes the same viewport size through a regular InheritedWidget, transparent to intrinsic queries, while SingleChildScrollView already forwards cross-axis intrinsic queries to its child. Net effect: vertical alignment in scrollable Page/View still works (the original #6450 intent), and IntrinsicWidth/Height ancestors are no longer broken. Restores scroll=AUTO on the alignment example to validate the fix.
…yBoxes PR #6450 used a LayoutBuilder to read the parent's constraints and apply the viewport's max-extent as a min-height on the scroll-view child, so vertical alignment works in scrollable Page/View when content is shorter than the viewport. LayoutBuilder, however, reports 0 for intrinsic dimensions, which collapses any ancestor IntrinsicWidth / IntrinsicHeight and leaves the layout perpetually dirty (test_alignment was tripping !debugNeedsPaint in toImage()). Replace the LayoutBuilder with two cooperating RenderProxyBoxes that forward intrinsic queries to their child: * _OuterConstraintsReader sits outside Scrollbar/SingleChildScrollView and captures its incoming constraints during performLayout. * _InnerConstraintsEnforcer sits inside SingleChildScrollView and reads the captured value back, applying it as a min on the scroll-view child. Same render output as the original LayoutBuilder (including correct no-op behavior for nested-in-unbounded-scroll cases), but transparent to ancestor IntrinsicWidth/Height.
…t change The inner enforcer lives inside SingleChildScrollView, which provides unbounded constraints in the scroll axis to its child. On window resize those constraints don't change, so Flutter's layout pipeline skips the inner enforcer's performLayout — and it keeps applying the stale min captured on first layout. Result: vertical alignment looks correct initially but freezes at the original viewport size after a resize. Track the inner enforcer in the constraints holder; when the outer reader sees its incoming constraints change, mark the inner dirty via invokeLayoutCallback (which enables mutations during layout). The inner then re-runs performLayout later in the same pass with the updated holder value.
Revert the temporary narrowing that scoped runs to test_row.py while diagnosing the !debugNeedsPaint regression introduced by PR #6450.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #6460
Summary by Sourcery
Fix NavigationBarDestination selected icon rendering when provided as a control and update tests and changelog accordingly.
Bug Fixes:
Documentation:
Tests: